Use DefaultAzureCredential by default (#497)#550
Use DefaultAzureCredential by default (#497)#550janjagusch wants to merge 4 commits intodrivendataorg:masterfrom
Conversation
janjagusch
force-pushed
the
feat/default-azure-credential
branch
2 times, most recently
from
03ea50f to
e23d7e5
Compare
…provided When `account_url` is provided without `credential`, automatically use `DefaultAzureCredential` from `azure-identity` if installed, bringing Azure auth in line with how `GSClient` uses `google.auth.default()`. Also adds support for `AZURE_STORAGE_ACCOUNT_URL` env var as a fallback. Closes drivendataorg#497 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
janjagusch
force-pushed
the
feat/default-azure-credential
branch
from
e23d7e5 to
fafa0b7
Compare
|
@pjbull, hope you don't mind this agent-written PR. I reviewed the changes myself, making sure the diff remains minimal and plausible, and the changes look good to me. :) |
|
Friendly ping, @pjbull. Let me know what you think of this PR. :) |
|
@pjbull, friendly reminder. :) Is there anything I can do to move this PR forward? |
pjbull
left a comment
pjbull
left a comment
There was a problem hiding this comment.
This is not sufficient review of an AI generated PR. Please don't submit AI PRs that don't follow the conventions of the repository, it creates an unnecessary burden for maintainers. We have extensive contributing docs.
Did you actually test this against a real Azure backend configured to use this path? A mock is not enough proof this works.
Also, please follow existing test patterns, not the MagicMock patterns in this PR. You should add azure mocks if you need them to the mock we already have and actually ensure the AzureClient object gets properties set on it correctly.
| try: | ||
| from azure.identity import DefaultAzureCredential | ||
| except ImportError: | ||
| DefaultAzureCredential = None |
There was a problem hiding this comment.
Please follow existing convention and add to import block of azure dependencies above.
There was a problem hiding this comment.
The azure.identity import was intentionally left out of the big import block above, as azure-identity is an optional azure dependency, meaning you can use the azure blob client without it (you will just not be able to use identity-based authentication).
I've left a comment to make this clearer. Let me know if you prefer a different approach.
Replace unittest.mock.MagicMock/patch patterns with the project's existing MockBlobServiceClient and MockedDataLakeServiceClient, following the same monkeypatch approach used in conftest.py. Tests now verify actual client properties instead of asserting on mock calls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
||
| [project.optional-dependencies] | ||
| azure = ["azure-storage-blob>=12", "azure-storage-file-datalake>=12"] | ||
| azure = ["azure-storage-blob>=12", "azure-storage-file-datalake>=12", "azure-identity>=1"] |
There was a problem hiding this comment.
Strictly speaking, we could factor out azure-identity into its own dependency group. But I didn't want to overcomplicate things.
2 participants
Summary
account_urlis provided withoutcredential, automatically usesDefaultAzureCredentialfromazure-identityif installed (falls back tocredential=Noneif not installed)AZURE_STORAGE_ACCOUNT_URLenvironment variable as a new fallback before raisingMissingCredentialsError, mirroring the existingAZURE_STORAGE_CONNECTION_STRINGpattern""AZURE_STORAGE_CONNECTION_STRING"andraised raised)Closes #497
Test plan
test_azureblobpath_nocredsupdated to also clearAZURE_STORAGE_ACCOUNT_URLenv varDefaultAzureCredentialused whenaccount_urlprovided withoutcredentialcredentialtakes precedence overDefaultAzureCredentialcredential=Nonewhenazure-identitynot installedAZURE_STORAGE_ACCOUNT_URLenv var with.blob.URLAZURE_STORAGE_ACCOUNT_URLenv var with.dfs.URLMissingCredentialsErrorstill raised with no configtest_azure_specific.pypasstest_local.pypass🤖 Generated with Claude Code